refactor(sdk): remove deprecated v1 split build/sign transaction surface - #4323
Merged
Conversation
Remove the concurrency-unsafe split fund-then-sign Core send path across the SDK stack, superseded by the atomic finalizers (finalize/v2 broadcast and the deferred signed-payment flow): - rs-platform-wallet-ffi: drop core_wallet_tx_builder_set_funding, core_wallet_tx_builder_build_signed and core_wallet_broadcast_transaction (with the now-dead as_standard_account_type / managed_account_mut helpers). FFICoreTransaction and core_wallet_transaction_free stay: they are the ownership-transfer mechanism of the v2 core_wallet_signed_payment_finalize out-params. - rs-unified-sdk-jni: drop the coreTxBuilderSetFunding, coreTxBuilderBuildSigned, coreWalletBroadcastTransaction and coreTransactionFree trampolines. - kotlin-sdk: drop the deprecated internal setFunding/buildSigned, the CoreTransaction class, the v1 broadcastTransaction(CoreTransaction) overload and the matching external funs. - swift-sdk: drop the deprecated public setFunding/buildSigned, the CoreTransaction type and both v1 broadcast entry points (broadcastTransactionWithOutcome/broadcastTransaction over CoreTransaction). The split path raced funding selection against signing across the C ABI (two concurrent same-account builds could select the same UTXO); the atomic finalizers commit selection + reservation under one wallet-manager lock. An audit found no remaining in-repo call sites; dashwallet-ios is migrated to finalizeAtomic separately before this lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
August 6, 2026 18:16
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change removes legacy split transaction-building, transaction ownership, and broadcast APIs. Rust FFI, JNI, Kotlin, and Swift now use atomic finalizers, V2 signed-transaction broadcasting, deferred payment handling, and finalized-transaction abandonment. ChangesAtomic finalization and broadcast migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedCoreWallet
participant FinalizedCoreTransaction
participant core_wallet_broadcast_signed_transaction_v2
ManagedCoreWallet->>FinalizedCoreTransaction: consume finalized transaction
ManagedCoreWallet->>core_wallet_broadcast_signed_transaction_v2: broadcast signed transaction
core_wallet_broadcast_signed_transaction_v2-->>ManagedCoreWallet: return broadcast outcome
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
|
🔍 Review in progress — actively reviewing now (commit 2f2cd07) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt (1)
202-202: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the V2 transaction-handle term.
coreTxBuilderFinalizereturns an opaque signed-transaction V2 handle. It does not return the deferred-payment registry token. The current text can cause callers to apply the wrong ownership model.Proposed fix
- /** Atomic V2 finalizer; consumes [builder] and returns an opaque registry handle. */ + /** Atomic V2 finalizer; consumes [builder] and returns an opaque signed-transaction V2 handle. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt` at line 202, Update the documentation comment for coreTxBuilderFinalize to describe its result as an opaque signed-transaction V2 handle, not a registry handle or deferred-payment token. Keep the note that the operation is an atomic V2 finalizer consuming builder.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt`:
- Line 202: Update the documentation comment for coreTxBuilderFinalize to
describe its result as an opaque signed-transaction V2 handle, not a registry
handle or deferred-payment token. Keep the note that the operation is an atomic
V2 finalizer consuming builder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7441ccc8-c5b5-4702-a2df-2aebbd023cc6
📒 Files selected for processing (13)
packages/kotlin-sdk/KotlinExampleApp/TEST_PLAN.mdpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rspackages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rspackages/rs-unified-sdk-jni/src/wallet_manager.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
💤 Files with no reviewable changes (2)
- packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift
thepastaclaw
reviewed
Aug 6, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex + Opus
The deprecated split transaction surface is removed consistently across Rust FFI, JNI, Kotlin, and Swift, with the retained V2 and deferred-finalization ownership paths remaining intact. No correctness, security, ABI, dependency-direction, or consensus issues were verified; two in-scope documentation nitpicks remain: stale JNI references to the deleted API and an introduced source-formatting regression in a Rust doc comment.
Source: reviewer backend models gpt-5.6-sol and claude-sonnet-5; final verifier backend model gpt-5.6-sol, used as the permitted fallback after both claude-sonnet-5 verifier attempts produced unparseable output. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— final-verifier (Opus verifier fallback) - Opus reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— security-auditor (completed)
💬 2 nitpick(s)
1 additional finding(s) omitted (not in diff).
Comment on lines
78
to
80
| /// Selection and insertion into the account ReservationSet happen under one | ||
| /// wallet-manager lock, so they cannot interleave with a competing finalizer. The wallet-manager lock is dropped before the host | ||
| /// mnemonic resolver is invoked. This function consumes `builder` on every |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Restore the source wrapping in the finalizer documentation
The updated comment joins the end of one sentence and the start of the next on a single unusually long source line. Rustdoc rendering is unaffected, but this PR introduced a source-formatting regression while removing the deprecated API reference, and the line no longer follows the wrapping style used by the surrounding documentation.
Suggested change
| /// Selection and insertion into the account ReservationSet happen under one | |
| /// wallet-manager lock, so they cannot interleave with a competing finalizer. The wallet-manager lock is dropped before the host | |
| /// mnemonic resolver is invoked. This function consumes `builder` on every | |
| /// Selection and insertion into the account ReservationSet happen under one | |
| /// wallet-manager lock, so they cannot interleave with a competing finalizer. | |
| /// The wallet-manager lock is dropped before the host mnemonic resolver is | |
| /// invoked. This function consumes `builder` on every path after its pointer |
source: ['claude']
Restore the wrapped line the removal edit joined in the core_wallet_tx_builder_finalize doc, and describe coreTxBuilderFinalize's result as a signed-transaction V2 handle rather than a registry handle (the registry token belongs to the deferred signed-payment flow). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
The v1 split Core-send path (
set_funding→build_signed→ broadcast) is concurrency-unsafe by construction: funding selection and signing are split across the C ABI, so the wallet-manager lock cannot be held across the boundary and two concurrent same-account builds can select the same UTXO (a double-spend at broadcast). The atomic finalizers (#4185 and the deferred flow in #4308/#4322) were built to close this, every SDK convenience path already routes through them, and an audit found zero remaining in-repo call sites — the split path survived only as exported API/ABI, where keeping it publicly reachable (especially in Swift) invites misuse.What was done?
Removed the v1 surface across the stack:
core_wallet_tx_builder_set_funding,core_wallet_tx_builder_build_signed,core_wallet_broadcast_transaction, plus the now-deadCoreAccountTypeFFI::as_standard_account_type/managed_account_muthelpers.FFICoreTransactionandcore_wallet_transaction_freeare kept: they are the ownership-transfer mechanism of the v2core_wallet_signed_payment_finalizeout-params (feat(swift-sdk): split build/broadcast with reservation release for BIP70-style deferred submission #4322), so they are no longer v1-only.coreTxBuilderSetFunding,coreTxBuilderBuildSigned,coreWalletBroadcastTransaction,coreTransactionFreetrampolines.setFunding/buildSigned, theCoreTransactionclass, the v1broadcastTransaction(CoreTransaction)overload, and the matching external funs.setFunding/buildSigned, theCoreTransactiontype, and both v1 broadcast entry points (broadcastTransactionWithOutcome(_: CoreTransaction)and the deprecated throwing wrapper).The parity manifest has no shared_symbols entries for the removed functions (verified).
dashwallet-ios was the one out-of-repo consumer; it is migrated to
finalizeAtomicondashwallet-iosbranchfeat/migrate-v2-atomic-send, which should land before the platform pin there advances past this PR.How Has This Been Tested?
cargo check/cargo clippy --all-targets/cargo fmtonplatform-wallet-ffi,rs-unified-sdk-jni,rs-sdk-ffi(clippy: only pre-existing warnings in untouchedpersistence.rs)cargo test -p platform-wallet-ffi: 235 + 26 + 6 pass:sdk:compileDebugKotlin,:sdk:compileDebugAndroidTestKotlin,:sdk:testDebugUnitTestall greenswift build+SignedCoreTransactionTests(5 tests) green against a freshly built xcframework at this commitBreaking Changes
None consensus-level. Pre-release SDK API removal; the replacement atomic surface has been in place since #4185/#4322.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit